Skip to content

Improve PID parameter validation #510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Oct 14, 2025
Merged

Improve PID parameter validation #510

merged 6 commits into from
Oct 14, 2025

Conversation

@christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented Oct 13, 2025 •
edited
Loading

@christophfroehlich christophfroehlich added the backport-jazzy Triggers PR backport to ROS 2 jazzy. label Oct 13, 2025
@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 84.21053% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.69%. Comparing base (b37cf44) to head (e74ec11).

Files with missing lines Patch % Lines
control_toolbox/include/control_toolbox/pid.hpp 57.14% 0 Missing and 3 partials ⚠️
Additional details and impacted files 
@@               Coverage Diff               @@
##           ros2-master     #510      +/-   ##
===============================================
+ Coverage        82.35%   82.69%   +0.33%     
===============================================
  Files               29       29              
  Lines             1984     1982       -2     
  Branches           114      113       -1     
===============================================
+ Hits              1634     1639       +5     
+ Misses             281      274       -7     
  Partials            69       69
Flag Coverage Δ
unittests 82.69% <84.21%> (+0.33%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ontrol_toolbox/include/control_toolbox/pid_ros.hpp 100.00% <ø> (ø)
control_toolbox/src/pid.cpp 83.73% <100.00%> (ø)
control_toolbox/src/pid_ros.cpp 83.77% <100.00%> (ø)
control_toolbox/test/pid_ros_parameters_tests.cpp 99.02% <100.00%> (+0.02%) ⬆️
control_toolbox/test/pid_tests.cpp 100.00% <ø> (ø)
control_toolbox/include/control_toolbox/pid.hpp 65.75% <57.14%> (+4.77%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@christophfroehlich
Copy link
Contributor Author

@bijoua29 @ViktorCVS maybe you can have a quick look please, do you agree?

@mergify mergify bot mentioned this pull request Oct 14, 2025
Merged
bijoua29
bijoua29 reviewed Oct 14, 2025
View reviewed changes
Copy link
Contributor

@bijoua29 bijoua29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you checked if i_min=i_max=0, that it is internally consistent?

@christophfroehlich
Copy link
Contributor Author

Have you checked if i_min=i_max=0, that it is internally consistent?

From: https://en.cppreference.com/w/cpp/algorithm/clamp.html

If lo is greater than hi, the behavior is undefined.

If it equals, then std::clamp will just force it to zero, right?

bijoua29
bijoua29 suggested changes Oct 14, 2025
View reviewed changes
@christophfroehlich christophfroehlich merged commit 84a0c5c into ros2-master Oct 14, 2025
25 checks passed
@christophfroehlich christophfroehlich deleted the improve/pid/validation branch October 14, 2025 20:31
mergify bot pushed a commit that referenced this pull request Oct 14, 2025
@christophfroehlich @mergify
Improve PID parameter validation (#510)
f3b0b0e 
(cherry picked from commit 84a0c5c)

# Conflicts:
#	control_toolbox/include/control_toolbox/pid.hpp
#	control_toolbox/include/control_toolbox/pid_ros.hpp
#	control_toolbox/src/pid_ros.cpp
#	control_toolbox/test/pid_ros_parameters_tests.cpp
@mergify mergify bot mentioned this pull request Oct 14, 2025
Merged
christophfroehlich added a commit that referenced this pull request Oct 14, 2025
@christophfroehlich
Improve PID parameter validation (#510)
1f4772d
christophfroehlich pushed a commit that referenced this pull request Oct 15, 2025
@mergify
Improve PID parameter validation (#510) (#515)
ec53758
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ARK3r ARK3r Awaiting requested review from ARK3r

@bmagyar bmagyar Awaiting requested review from bmagyar

@homalozoa homalozoa Awaiting requested review from homalozoa

@qiayuanl qiayuanl Awaiting requested review from qiayuanl

@rosterloh rosterloh Awaiting requested review from rosterloh

@saikishor saikishor Awaiting requested review from saikishor

1 more reviewer

@bijoua29 bijoua29 bijoua29 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport-jazzy Triggers PR backport to ROS 2 jazzy.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@christophfroehlich @codecov-commenter @bijoua29